The Details: section of the error message includes nodeInfo() at the end. However, the nodeInfo() is already included in the overall error message. Consider removing the redundant nodeInfo() call to avoid repetition.

            String.format("Error retrieving %s: %s. Details: %s",
                toContextString(),
                status.getCode(),
                status.getDescription() != null ? status.getDescription() : "")

This block translates the Status from the update to a translatedStatus before creating a StatusOr from it. It would be helpful to include the original status as the cause of the translatedStatus exception, to preserve the original error information for debugging purposes.

Also, the status.getDescription() can be null, so it's good that you are handling that.

@gemini-code-assist so we don't even need to store in data? Because you suggested to remove this line as well.
data = StatusOr.fromStatus(translatedStatus);

No, the line data = StatusOr.fromStatus(translatedStatus); should definitely remain. My apologies if my previous suggestion was unclear or misleading in its scope. The purpose of that line is to propagate the translatedStatus to the data field, which is crucial for the watcher to correctly handle the resource error.

My intention was only to enhance the translatedStatus itself by including the original status as a cause, which aids in debugging by preserving the full error context.

Here is a refined suggestion that only modifies the translatedStatus creation, assuming status is already defined on line 654:

This logic seems to be checking if the ADS stream closed with an OK status before receiving a response, and if so, it's setting the statusToPropagate to UNAVAILABLE. It would be helpful to add a comment explaining why an OK status in this scenario is considered a connectivity error, referencing gRFC A57 for context.

It would be helpful to add a comment explaining why the statusOr is being captured, and why the hasValue() check is being performed.

    verify(ldsWatcher2, timeout(5000)).onResourceChanged(ldsUpdateCaptor.capture());
    StatusOr<LdsUpdate> statusOr = ldsUpdateCaptor.getValue();
    assertThat(statusOr.hasValue()).isTrue(); // Add comment here
    assertThat(statusOr.getValue()).isEqualTo(

It seems like the changes introduced flakyness for the below assert, so it's being commented out. It would be helpful to add a comment explaining why this assert is flaky, and what conditions cause it to fail.

This assertion was commented out, which reduces test coverage. The flakiness is likely due to the change in BootstrapperImpl where ignoreResourceDeletion now defaults to xdsDataErrorHandlingEnabled, a value influenced by an environment variable. To ensure test predictability, please fix this by controlling the environment variable within the test or by mocking GrpcUtil.getFlag.

@gemini-code-assist can you help me fix this?

It seems the flakiness you observed might be due to the xds.data_error_handling.enabled system property or environment variable being set to true in some test environments, which could influence the ignoreResourceDeletion flag. Although the GrpcBootstrapperImpl constructor explicitly sets this to false in this test, it's good practice to ensure test isolation by explicitly controlling such flags.

To resolve the flakiness and re-enable the assertion, you can wrap the relevant part of your test method in a try-finally block to set and restore the system property. This ensures that the test runs with a predictable value for xds.data_error_handling.enabled.

Here's how you can modify the useV2ProtocolByDefault and useV3ProtocolIfV3FeaturePresent test methods:

  @Test
  public void useV2ProtocolByDefault() throws XdsInitializationException {
    String originalFlag = System.getProperty("xds.data_error_handling.enabled");
    try {
      System.setProperty("xds.data_error_handling.enabled", "false");
      BootstrapInfo info = buildBootstrapInfo(
          "{\n"
              + "  \"xds_servers\": [{\n"
              + "    \"server_uri\": \"" + SERVER_URI + "\",\n"
              + "    \"channel_creds\": [\n"
              + "      {\"type\": \"insecure\"}\n"
              + "    ]\n"
              + "  }]\n"
              + "}");
      ServerInfo serverInfo = Iterables.getOnlyElement(info.servers());
      assertThat(serverInfo.target()).isEqualTo(SERVER_URI);
      assertThat(serverInfo.implSpecificConfig()).isInstanceOf(InsecureChannelCredentials.class);
      assertThat(serverInfo.ignoreResourceDeletion()).isFalse();
    } finally {
      if (originalFlag == null) {
        System.clearProperty("xds.data_error_handling.enabled");
      } else {
        System.setProperty("xds.data_error_handling.enabled", originalFlag);
      }
    }
  }

I've provided a code suggestion below to uncomment the assertion. Please apply the try-finally block as shown above to both affected test methods (useV2ProtocolByDefault and useV3ProtocolIfV3FeaturePresent) to ensure consistent test results.

These assertions in used_then_mainServerRestart_fallbackServerUp were commented out due to flakiness. This compromises test coverage for fallback behavior. The flakiness might stem from race conditions during server restart and client reconnection. Instead of disabling the checks, please investigate the root cause. The test could be made more robust, for instance, by using verify with a longer timeout or waiting for a specific state before making assertions.

@gemini-code-assist can you help me fix this?

To address the flakiness in used_then_mainServerRestart_fallbackServerUp, we need to ensure the test waits sufficiently for the expected state changes after the main server restarts and the client reconnects.

1. The assertion assertThat(cdsUpdateCaptor.getValue().getStatus().isOk()).isTrue() checks if the received CDS update is successful. If this was flaky, it might indicate that an error update was sometimes captured before a successful one, or that the client isn't consistently receiving a successful update from the main server after restart. We will uncomment this assertion.

2. The assertion assertThat(getLrsServerInfo("localhost:" + fallbackServer.getServer().getPort())).isNull() checks that the client is no longer reporting Load Reporting Service (LRS) information to the fallback server. This state change might not be instantaneous after the client switches back to the main server. We should explicitly advance the fakeClock to allow time for this state to settle before making the assertion.

Here's a suggested change to make these assertions more robust:

@gemini-code-assist I want you to review the XdsDependencyManager and tell me the use of variable 'ambientError'. As per gRFC A88. Right now I'm only focused on implementing xds client watcher API changes.

The comment on line 478 states that the handler is notified before cleaning up the stream state. However, close(status.asException()) is called on line 475, which cleans up the stream state by setting adsStream to null. To align the code with the comment's intent and avoid potential race conditions or unexpected behavior if handleStreamClosed were to depend on the stream's state, it's better to notify the handler before closing the stream.

The logic for determining ignoreResourceDeletion seems to enable the feature if xdsDataErrorHandlingEnabled is true, even if the server does not send the ignore_resource_deletion feature. This appears to contradict gRFC A88, which states: "If this feature is not present, then the client MUST NOT ignore resource deletions."

To align with the gRFC, ignoreResourceDeletion should only be enabled when the server explicitly signals support via the ignore_resource_deletion feature. The current implementation could lead to incorrect behavior where the client ignores resource deletions when the server does not expect it to.

I suggest changing this line to initialize ignoreResourceDeletion to false. This ensures that it will only be enabled if the server feature is present in the if block below.

In onAmbientError, error.getDescription() can be null, which would result in the string "null" being embedded in the error message. This is inconsistent with how null descriptions are handled in onResourceChanged (lines 656-661), where a null description is replaced with an empty string.

For consistency and clearer error messages, I suggest handling the potentially null description in the same way.

@gemini-code-assist what is the use of ambientError variable? Why we are not doing anything in onAmbientError here?

That's a great question! The ambientError variable and the onAmbientError method are introduced as part of the XdsClient watcher API changes, specifically detailed in gRFC A88.

1. Use of ambientError variable:

   • The ambientError variable (declared on line 637 in the XdsDependencyManager.java file) is intended to store transient errors that occur in the xDS client. These are errors that provide useful information about the general state of the XdsClient but should not cause the watcher to discard its currently active, valid resource.
   • It's marked with @SuppressWarnings("unused") because, in this specific XdsWatcherBase class, its primary purpose is to be set and potentially retrieved by other components or for logging/metrics, rather than directly triggering an action within this base class itself. The actual use of this ambient error information would typically happen in concrete implementations or higher-level logic that consumes the XdsWatcherBase.

2. Why nothing is done in onAmbientError (to change the resource state):

   • As per gRFC A88, onAmbientError is designed for errors that "should not modify the watcher's use of that resource but that may be useful information about the ambient state of the XdsClient." This means that unlike onResourceChanged (which can deliver an error that does invalidate the current resource), onAmbientError explicitly should not cause the watcher to stop using its previously received valid resource.
   • Therefore, the implementation of onAmbientError in XdsDependencyManager.java correctly captures the error in ambientError and formats a log message, but it does not update the data field (which holds the active resource) or call maybePublishConfig(), because doing so would imply a change in the active resource, which is contrary to the definition of an "ambient" error.

My previous suggestion was to ensure that even when an ambient error's description is null, it's handled gracefully (by replacing null with an empty string) to prevent the literal string "null" from appearing in the formatted error message, improving clarity and consistency.

There is a potential race condition here. The watcher lists (rdsCopy, cdsCopy, edsCopy) are created by iterating over rdsWatchers.values(), etc., outside the syncContext.execute() block. However, the underlying maps (rdsWatchers, cdsWatchers, edsWatchers) can be modified concurrently by other operations within the syncContext (e.g., cancelXdsResourceWatch). This could lead to a ConcurrentModificationException or other concurrency issues.

To ensure thread safety, the creation of these lists should be moved inside the syncContext.execute() block.

The test isolatedResourceDeletions seems to be missing a verification. It correctly verifies that mockDirectPathWatcher (for the resource that is not deleted) is not affected by the deletion. However, it no longer verifies that mockWatcher (for the deleted resource) receives a deletion notification. The old test had verify(mockWatcher, timeout(20000)).onResourceDoesNotExist("test-server");. With the new API, this should be a call to onResourceChanged with a NOT_FOUND status. It would be good to add this verification back to ensure the deletion notification is correctly propagated to the right watcher.

@gemini-code-assist help me with the fix.

You're right to ask for help with this fix! To properly verify the deletion notification for mockWatcher using the new API, you should check for an onResourceChanged call with a NOT_FOUND status, and ensure the description contains the resource name.

Here's the suggested code to replace lines 132-133, which adds the missing verification for mockWatcher while retaining the existing verifications for mockDirectPathWatcher:

The test filterState_shutdown_onLdsNotFound now tests behavior for deliverLdsResourceDeletion(), which calls onAmbientError. An ambient error correctly does not shut down filters. However, the test name is now misleading as it suggests it's testing a "not found" scenario that leads to a shutdown. Please consider renaming this test to reflect that it's testing an ambient error scenario, for example, filterState_noShutdown_onLdsDeletion.

@gemini-code-assist Is this test correct logically? Should we change the test? Why is the function of test changed altogether? Does filterState_shutdown_onLdsNotFound doesnt hold true at all with new APIs? Should we create a new test for filterState_noShutdown_onLdsDeletion?

Thanks for the questions, shivaspeaks! Let me clarify the situation regarding the test filterState_shutdown_onLdsNotFound.

The core of the change in this Pull Request is the update to the XdsClient.ResourceWatcher API. Previously, there were three methods:

• onChanged(T update): For successful resource updates.
• onError(Status error): For transient errors.
• onResourceDoesNotExist(String resourceName): For definitive resource not found scenarios.

This PR introduces a new API with two methods:

• onResourceChanged(StatusOr<T> update): This single method now handles both successful updates (when update.hasValue() is true) and definitive resource errors like NOT_FOUND (when update.hasValue() is false and update.getStatus() indicates an error).
• onAmbientError(Status error): This is for transient errors that should not cause the watcher to stop using a previously received valid resource. The gRFC A88 states: "the watcher should not stop using the previously seen resource, and the XdsClient will not remove the resource from its cache."

Now, let's address your specific questions:

1. Is this test correct logically?

   • The assertions in the modified test (assertThat(lds1Filter1.isShutdown()).isFalse(); and assertThat(lds1Filter2.isShutdown()).isFalse();) are logically correct for the action being performed (xdsClient.deliverLdsResourceDeletion()). This action, as per the new API, triggers onAmbientError, which should indeed not shut down the filters.
   • However, the test name filterState_shutdown_onLdsNotFound is now misleading. It implies a "resource not found" scenario that leads to a shutdown, but the code is testing an "ambient error" scenario that does not lead to a shutdown.

2. Should we change the test?

   • Yes, the test should be updated to align its name with the behavior it's testing, or to test the scenario implied by its name.

3. Why is the function of test changed altogether?

   • The function of the test changed because xdsClient.deliverLdsResourceNotFound() (which previously caused a shutdown) was replaced with xdsClient.deliverLdsResourceDeletion(). As explained above, deliverLdsResourceDeletion() now maps to the new onAmbientError() API, which has different semantics regarding resource shutdown compared to the old onResourceDoesNotExist() (which deliverLdsResourceNotFound() used to call).

4. Does filterState_shutdown_onLdsNotFound doesn't hold true at all with new APIs?

   • The concept of a "resource not found" leading to a shutdown still holds true with the new APIs. The equivalent of the old onResourceDoesNotExist() is now handled by onResourceChanged(StatusOr.fromStatus(NOT_FOUND)). This event should cause a shutdown of the filters.
   • The current test, however, is testing the onAmbientError path, which explicitly states it should not cause a shutdown.

5. Should we create a new test for filterState_noShutdown_onLdsDeletion?

   • Yes, I recommend creating a new test for the onAmbientError scenario, perhaps named filterState_noShutdown_onLdsDeletion as suggested. This new test would correctly use xdsClient.deliverLdsResourceDeletion() and assert that the filters are not shut down.
   • The existing test filterState_shutdown_onLdsNotFound should then be reverted to its original intent: it should call xdsClient.deliverLdsResourceNotFound() (which now correctly triggers onResourceChanged(StatusOr.fromStatus(NOT_FOUND))) and assert that the filters are shut down (isTrue()). This would ensure both important scenarios are covered and clearly named.

@gemini-code-assist then help me with the test for both filterState_noShutdown_onLdsDeletion and filterState_shutdown_onLdsNotFound.

You're right, shivaspeaks! I can certainly help with that. Based on our previous discussion, the goal is to:

1. Create a new test filterState_noShutdown_onLdsDeletion to specifically test the scenario where deliverLdsResourceDeletion() (which maps to onAmbientError) does not shut down the filters. This test will essentially take the logic currently present in your filterState_shutdown_onLdsNotFound method.
2. Restore the original intent of filterState_shutdown_onLdsNotFound to test the scenario where deliverLdsResourceNotFound() (which maps to onResourceChanged(StatusOr.fromStatus(NOT_FOUND))) does shut down the filters.

Here's the suggested new test method filterState_noShutdown_onLdsDeletion. You can add this method after your existing filterState_shutdown_onLdsNotFound method:

  @Test
  public void filterState_noShutdown_onLdsDeletion() throws Exception {
    StatefulFilter.Provider lds1Filter1 = mock(StatefulFilter.Provider.class);
    StatefulFilter.Provider lds1Filter2 = mock(StatefulFilter.Provider.class);
    filterStateTestSetupResolver(lds1Filter1, lds1Filter2);

    // LDS 2: resource deletion (ambient error).
    reset(mockListener);
    when(mockListener.onResult2(any())).thenReturn(Status.OK);
    xdsClient.deliverLdsResourceDeletion();
    verify(mockListener, never()).onResult2(any());
    // Verify no shutdown.
    assertThat(lds1Filter1.isShutdown()).isFalse();
    assertThat(lds1Filter2.isShutdown()).isFalse();
  }

And here is the code_suggestion to modify your existing filterState_shutdown_onLdsNotFound method (lines 1561-1569) to correctly test the resource not found scenario that leads to shutdown. This restores its original purpose with the new API semantics:

Similar to the previous comment, the test filterState_shutdown_onRdsNotFound now tests an ambient error scenario by calling deliverAmbientError. The test name is misleading. Please consider renaming it to something like filterState_noShutdown_onRdsAmbientError to better reflect its purpose.

@gemini-code-assist just like previous comment, help me write both tests filterState_shutdown_onRdsNotFound and filterState_noShutdown_onRdsAmbientError

Certainly! You're right, having both tests will improve clarity and coverage.

First, please rename the current test method filterState_shutdown_onRdsNotFound (starting at line 1617) to filterState_noShutdown_onRdsAmbientError. The existing code within this method is already correctly testing the scenario where filters are not shut down on an ambient error.

Next, here is the code for the new filterState_shutdown_onRdsNotFound test method. This test verifies that filters are shut down when an RDS resource is definitively not found. You can add this method after the filterState_noShutdown_onRdsAmbientError method.

  @Test
  public void filterState_shutdown_onRdsNotFound() {
    StatefulFilter.Provider filterProvider = filterStateTestSetupResolver();
    StatefulFilter lds1Filter1 = filterProvider.create("filter1");
    StatefulFilter lds1Filter2 = filterProvider.create("filter2");

    // Initial setup to get filters active
    xdsClient.deliverLdsUpdateWithFilters(
        VirtualHost.create("virtual-host", Collections.singletonList("domain"),
            Collections.singletonList(Route.create(
                RouteMatch.create(null, null, null),
                RouteAction.newBuilder().setCluster(RDS_RESOURCE_NAME).build()))),
        ImmutableList.of(
            new NamedFilterConfig("filter1", lds1Filter1),
            new NamedFilterConfig("filter2", lds1Filter2)));
    xdsClient.deliverRdsUpdate(RDS_RESOURCE_NAME,
        Collections.singletonList(VirtualHost.create("virtual-host",
            Collections.singletonList("domain"),
            Collections.singletonList(Route.create(
                RouteMatch.create(null, null, null),
                RouteAction.newBuilder().setCluster("cluster-name").build())))));
    assertResolutionResult(expectedLdsResourceName, "cluster-name");
    assertThat(lds1Filter1.isShutdown()).isFalse();
    assertThat(lds1Filter2.isShutdown()).isFalse();

    // RDS 2: RDS_RESOURCE_NAME not found (definitive error).
    reset(mockListener);
    when(mockListener.onResult2(any())).thenReturn(Status.OK);
    xdsClient.deliverRdsResourceNotFound(RDS_RESOURCE_NAME);
    assertEmptyResolutionResult(RDS_RESOURCE_NAME);
    assertThat(lds1Filter1.isShutdown()).isTrue();
    assertThat(lds1Filter2.isShutdown()).isTrue();
  }

This change is correct, but it introduces a small performance regression by always creating a new ArrayList, even when defaultFilterChain is null. The previous implementation was more efficient as it only created a new list when necessary. Consider reverting to a pattern similar to the original implementation to avoid the unnecessary allocation, for example:

List<FilterChain> allFilterChains = filterChains;
if (defaultFilterChain != null) {
  allFilterChains = new ArrayList<>(filterChains);
  allFilterChains.add(defaultFilterChain);
}